[hmac, rtl] Do not skip padding after a hash stop command #25590
Conversation
andrea-caforio
force-pushed
the
hmac-disallow-padding-cancellation
branch
from
2e5c716 to
f2b230b
Compare
Thank you for the review @rswarbrick. You're right, the UNR file should be updated too. Unfortunately, we cannot generate these exclusion files with our VCS license on the Zurich workstation. |
andrea-caforio
requested review from
eshapira
and removed request for
a team
There was a problem hiding this comment.
This looks good to me from an RTL perspective. Thanks for the PR @andrea-caforio !
Did you run a full regression for this and inspect the coverage yet?
andrea-caforio
force-pushed
the
hmac-disallow-padding-cancellation
branch
from
f2b230b to
8d79a9b
Compare
Thank you @vogelpi for the review. I ran a full regression and all the transitions are now properly covered (see screenshot). I further added a second commit that updates the exclusion file with the list in #24692. @martin-velay you might want to double-check this.
|
|
Hi @andrea-caforio, I would prefer to have the exclusions in a separate PR as there is no direct link between this RTL fix and the exclusions. Thanks in advance ! |
andrea-caforio
force-pushed
the
hmac-disallow-padding-cancellation
branch
from
8d79a9b to
54425d2
Compare
Here you go #25696. |
|
CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_sha2_pad.sv This PR removes some undesired FSM transitions that were previously uncovered. This is okay. |
hw/ip/prim/rtl/prim_sha2_pad.sv
Outdated
| st_d = StIdle; | ||
| // We do not allow the cancellation of an ongoing padding operation, i.e., reverting back to the | ||
| // `StFifoReceive` state while being in the states `StPad80`, `StPad00`, `StLenHi` or `StLenLo`. | ||
| end else if (hash_go && (st_q == StIdle || st_q == StFifoReceive)) begin |
There was a problem hiding this comment.
| end else if (hash_go && (st_q == StIdle || st_q == StFifoReceive)) begin | |
| end else if (hash_go && (st_q inside {StIdle, StFifoReceive})) begin |
would be a bit more concise, but please don't consider this a required change
There was a problem hiding this comment.
I like this concise syntax. I pushed the change.
|
CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_sha2_pad.sv This PR removes some undesired FSM transitions that were previously uncovered. This is okay. |
This commit removes some conditional FSM states transition that were deemed undesirable in `prim_sha2_pad`, see lowRISC#23936. They made it possible to cancel an ongoing padding operation, which is disallowed by the HMAC module. This change makes it impossible to transition into the `StFifoReceive` state from either `StPad80`, `StPad00`, `StLenHi` or `StLenLo` by asserting the `hash_go` signal. Signed-off-by: Andrea Caforio <[email protected]>
andrea-caforio
force-pushed
the
hmac-disallow-padding-cancellation
branch
from
54425d2 to
23bbd40
Compare
Remove obsolete exclusions (see lowRISC#25590) and add two new exclusions regarding uncoverable "missing else" branches (see lowRISC#24692). Signed-off-by: Andrea Caforio <[email protected]>
This commit removes some conditional FSM states transition that were deemed undesirable in
prim_sha2_pad, see #23936. It made it possible to cancel an ongoing padding operation, which is disallowed by the HMAC module. As a side-effect, the removal plugs some existing FSM transition coverage holes inprim_sha2_padwhose manual exclusions can be removed as well.This change makes it impossible to transition into the
StFifoReceivestate from eitherStPad80,StPad00,StLenHiorStLenLoby asserting thehash_gosignal.